[sdf9] Support nested models in DOM and frame semantics #316

scpeters · 2020-07-17T01:56:20Z

Nested model elements (//model/model) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal, this PR adds support for nested models in the DOM API and frame semantics (fixing #283) through three steps:

adding Model::Model* methods for accessing nested models via the DOM API (047ec96)
loading nested models in Model::Load (b57fea2, step 3 of model parsing stages)
supporting nested models (//model/model) in frame semantics as opaque frames to match how models in the world scope (//world/model) are treated (85e0b4f, steps 6-9 of model parsing stages)

The first two steps are straightforward, while the 3rd step adds a bit of new behavior. I think it is reasonable to add this new behavior to libsdformat9 because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. Currently in libsdformat 9.2.0, a //world/model supports frame semantics: they have frames that can be reference by name with //pose/@relative_to and //world/frame/@attached_to values can resolve to a //world/model. This PR extends that same behavior to nested //model/model elements; they would have their own frames in the frame and pose graphs, and a //model/frame is permitted to resolve to a model.

This does not add support for referencing elements within a model via the :: syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see #293).

A companion pull request to sdf_tutorials will be submitted that documents the changes proposed in this pull request. This pull request bumps the minor version to 9.3. I believe we can make this behavior change because libsdformat9 has not been included as an official package in Ubuntu 20.04, so we don't have to worry about lots of people being stuck on an old version. Version 9.2 is currently in packages.ros.org, but I think we could get that version updated. (cc @j-rivero)

This change is

codecov-commenter · 2020-07-17T02:07:13Z

Codecov Report

Merging #316 into sdf9 will decrease coverage by 0.21%.
The diff coverage is 69.60%.

@@            Coverage Diff             @@
##             sdf9     #316      +/-   ##
==========================================
- Coverage   86.40%   86.18%   -0.22%     
==========================================
  Files          59       59              
  Lines        9053     9152      +99     
==========================================
+ Hits         7822     7888      +66     
- Misses       1231     1264      +33

Impacted Files	Coverage Δ
src/parser.cc	`77.43% <50.00%> (+0.02%)`	⬆️
src/FrameSemantics.cc	`76.23% <68.18%> (-0.99%)`	⬇️
src/Model.cc	`82.22% <69.81%> (-2.84%)`	⬇️
src/SemanticPose.cc	`100.00% <100.00%> (ø)`
src/World.cc	`93.12% <100.00%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99b7d9f...82b03f6. Read the comment docs.

EricCousineau-TRI · 2020-07-23T17:51:22Z

Reviewable link: https://reviewable.io/reviews/osrf/sdformat/316#-

EricCousineau-TRI

Reviewed 3 of 15 files at r1.
Reviewable status: 3 of 15 files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, @iche033, and @scpeters)

a discussion (no related file):
In the overview, could you perhaps add permalinks cross-referencing the proposal sections you're implementing?

(And when this merges, can you ensure the overview + PR number is included in the commit?)

EricCousineau-TRI

Reviewed 4 of 15 files at r1.
Reviewable status: 7 of 15 files reviewed, 2 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, @iche033, and @scpeters)

src/FrameSemantics.cc, line 557 at r1 (raw file):

  }

  // add nested model vertices and default edge if relative_to is empty

nit This graph construction relies heavily on the assumption of fully scoped / "unrolled" names.

Can you add a comment to that effect in this header file? (if it's not already there?)
(Just so it's easy to see the change once the scoping is implemented, if it's done in a more structure fashion?)

src/ign_TEST.cc, line 296 at r1 (raw file):

    std::string output =
      custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion);
    EXPECT_EQ("Valid.\n", output) << output;

BTW Woot!

EricCousineau-TRI

pending minor comments and request for negative test for drill-down

Reviewed 8 of 15 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @azeey, @iche033, and @scpeters)

test/sdf/model_nested_model_relative_to.sdf, line 1 at r1 (raw file):

<?xml version="1.0" ?>

Can you add a (brief) test that shows how drilling down will fail, and xref the issue? (#293)

test/sdf/model_nested_model_relative_to.sdf, line 2 at r1 (raw file):

<?xml version="1.0" ?>
<sdf version='1.7'>

nit Can you add a comment that this does not yet support drilling, and xref the issue? (#293)

azeey

LGTM! It would be good to add tests showing how sibling nested models can be used as joint parent/child frames when we merge this forward to sdf10. Specifically the case where the nested model is static might need attention.

Reviewed 5 of 15 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @iche033 and @scpeters)

src/FrameSemantics.cc, line 603 at r1 (raw file):

          "relative_to name[" + relativeTo +
          "] specified by link with name[" + link->Name() +
          "] does not match a link, joint, or frame name "

nit: Do we also need to add "nested model" here?

src/Model.cc, line 700 at r1 (raw file):

  if (!graph)
  {
    errors.push_back({ErrorCode::ELEMENT_INVALID,

Could the error code be made more specific to frame semantics? In Gazebo11, we ignore errors not related to frame semantics.

test/integration/model_dom.cc, line 256 at r1 (raw file):

  }
  EXPECT_TRUE(
    model->ModelByName("M1")->SemanticPose().Resolve(pose,

nit: this could be simplified to errors->empty()

iche033

I've been testing this PR with gazebosim/gz-sim#258 (mostly with SDF DOM but not frame semantics) and it's working fine.

One case of nested model that used to work in gazebo classic but no longer works now is:

<model>
  <model>
     <link />
  </model>
</model>

Here it complains that the top level model has no links. Do we want to support this use case? On the ign-gazebo and ign-physics side, they are already setup to work with canonical links inside nested models so that's taken care of. So just checking to see if this is possible on the frame semantics side?

EricCousineau-TRI · 2020-07-23T23:42:04Z

Oh... dern haha, we missed that entirely. I think we should make it work, b/c it makes sense.
Will file a PR to sdf_tutorials to try to capture this.

EricCousineau-TRI · 2020-07-23T23:52:39Z

Here's a (kinda messy) proposed change: gazebosim/sdf_tutorials#36

scpeters

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This graph construction relies heavily on the assumption of fully scoped / "unrolled" names.

Can you add a comment to that effect in this header file? (if it's not already there?)
(Just so it's easy to see the change once the scoping is implemented, if it's done in a more structure fashion?)

I'm not exactly sure what you mean by fully scoped / "unrolled" names

src/FrameSemantics.cc, line 603 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: Do we also need to add "nested model" here?

I updated this error message and several others in 7353fac

src/Model.cc, line 700 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

Could the error code be made more specific to frame semantics? In Gazebo11, we ignore errors not related to frame semantics.

I updated this error type along with a similar one in SemanticPose::Resolve in 61af87b

test/integration/model_dom.cc, line 256 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: this could be simplified to errors->empty()

instead I removed the print statement that I had added while debugging in 384c759

test/sdf/model_nested_model_relative_to.sdf, line 1 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you add a (brief) test that shows how drilling down will fail, and xref the issue? (#293)

I added an example file with a test that shows how drilling down will fail when specifying a nested link as a joint child in 33cfe4d. Should I add a similar example for :: in //pose/@relative_to?

scpeters

Reviewed 10 of 15 files at r1, 9 of 9 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

EricCousineau-TRI

Reviewed 6 of 9 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @azeey and @scpeters)

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

I'm not exactly sure what you mean by fully scoped / "unrolled" names

Ah, sorry. What I mean is that if I have two levels of nested models, e.g. Ma { Mb { Mc }}}, and for each model, I have L1 - J1 - L2, what should the names look like in this graph?

Should they always be absolute? e.g. Ma::Mb::Mc::L1, Ma::J1, etc.?
Or might they be relative? e.g. J1 in each respective scope?

I assume it's absolute. In that case, could a comment be added to FrameSemantics.hh stating that contract? e.g.

/// \warning All names passed to this graph must be *absolute*, e.g. `model::nested_model::my_frame`.

test/sdf/model_invalid_nested_joint_child.sdf, line 11 at r2 (raw file):

    <joint name="J" type="fixed">
      <parent>P</parent>
      <child>M::C</child>

Can you add a comment stating this will cause failure, and xref the issue (#293)?

test/sdf/model_nested_model_relative_to.sdf, line 1 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

I added an example file with a test that shows how drilling down will fail when specifying a nested link as a joint child in 33cfe4d. Should I add a similar example for :: in //pose/@relative_to?

Nah, I think that test is fine. Thanks!

test/sdf/model_nested_model_relative_to.sdf, line 2 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you add a comment that this does not yet support drilling, and xref the issue? (#293)

OK Retracting based on above discussion. (Moving to negative test.)

scpeters

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, sorry. What I mean is that if I have two levels of nested models, e.g. Ma { Mb { Mc }}}, and for each model, I have L1 - J1 - L2, what should the names look like in this graph?

Should they always be absolute? e.g. Ma::Mb::Mc::L1, Ma::J1, etc.?
Or might they be relative? e.g. J1 in each respective scope?

I assume it's absolute. In that case, could a comment be added to FrameSemantics.hh stating that contract? e.g.
/// \warning All names passed to this graph must be *absolute*, e.g. `model::nested_model::my_frame`.

there are no absolute names with :: in this PR; they are still all relative. each model has its own set of graphs using relative names and is treated as an opaque frame by whatever model or world contains it.

test/sdf/model_invalid_nested_joint_child.sdf, line 11 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you add a comment stating this will cause failure, and xref the issue (#293)?

added in 4e99441

EricCousineau-TRI

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

include/sdf/Model.hh, line 307 at r3 (raw file):

    /// \brief Give a weak pointer to the PoseRelativeToGraph to be used
    /// for resolving poses. This is private and is intended to be called by
    /// World::Load.

I believe this is called directly by things other than World::Load.

Can you update this to reflect that parent models will call this also?

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

there are no absolute names with :: in this PR; they are still all relative. each model has its own set of graphs using relative names and is treated as an opaque frame by whatever model or world contains it.

Gotcha. I looked at Model::SetPoseRelativeToGraph and see what you mean, as well as ModelPrivate::poseGraph, parentPoseGraph.

Can you indicate that this is the intended usage in FrameSemantics.hh, though?
e.g.

/// \ingroup sdf_frame_semantics
...
/// Note that all graphs should only contain relative names (e.g. "my_link"), not absolute names
/// ("top_model::nested_model::my_link"). Graphs across nested models
/// (currently via directly nested models in //model or //world elements) will be explicitly
/// separate graphs.
namespace sdf {
...
}

Then once we handle drilling down, we can revisit this assumption and explicitly show it in code diffs?

EricCousineau-TRI

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @azeey and @scpeters)

azeey

Reviewed 5 of 15 files at r1, 8 of 9 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scpeters)

EricCousineau-TRI

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @scpeters)

a discussion (no related file):
Per VC today, we think this should land once the proposal lands and is implemented for nested canonical links (perhaps in this PR?):
gazebosim/sdf_tutorials#36

scpeters

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)

include/sdf/Model.hh, line 307 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I believe this is called directly by things other than World::Load.

Can you update this to reflect that parent models will call this also?

ok, fixed in eee2f64

src/FrameSemantics.cc, line 557 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Gotcha. I looked at Model::SetPoseRelativeToGraph and see what you mean, as well as ModelPrivate::poseGraph, parentPoseGraph.

Can you indicate that this is the intended usage in FrameSemantics.hh, though?
e.g.
/// \ingroup sdf_frame_semantics
...
/// Note that all graphs should only contain relative names (e.g. "my_link"), not absolute names
/// ("top_model::nested_model::my_link"). Graphs across nested models
/// (currently via directly nested models in //model or //world elements) will be explicitly
/// separate graphs.
namespace sdf {
...
}
Then once we handle drilling down, we can revisit this assumption and explicitly show it in code diffs?

ok, fixed in eee2f64

EricCousineau-TRI · 2020-08-06T19:04:49Z

Per VC (again), the nested canonical link proposal may be implemented as part of this PR, or a preceding PR.

Update the spec to indicate that a frame may be attached to a //world/model or a nested //model/model. Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

Use POSE_RELATIVE_TO_GRAPH_ERROR instead of ELEMENT_INVALID when a valid PoseRelativeToGraph pointer is not available. Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

This syntax is unsupported by DOM objects in SDFormat 1.7. Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

Indicate that Model::Load can also call private method. State that only relative names are supported in graphs in FrameSemantics.hh Signed-off-by: Steve Peters <[email protected]>

scpeters

Reviewable status: 19 of 20 files reviewed, 3 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

In the overview, could you perhaps add permalinks cross-referencing the proposal sections you're implementing?

(And when this merges, can you ensure the overview + PR number is included in the commit?)

I've updated the pull request description with links to the modified parsing stages.

EricCousineau-TRI

Reviewed 3 of 9 files at r2, 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)

scpeters

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Per VC today, we think this should land once the proposal lands and is implemented for nested canonical links (perhaps in this PR?):
gazebosim/sdf_tutorials#36

I've started prototyping support for nested canonical links in libsdformat9 in scpeters@6c01718, but I'm not sure how much :: syntax we want to backport into SDFormat 1.7 (see the line in the proposal that says <model name="top" canonical_link="nested::link">).

I was very careful to avoid using :: syntax in gazebosim/sdf_tutorials#34, so I'd prefer to avoid it for the 1.7 implementation of gazebosim/sdf_tutorials#36 as well. :: syntax will be part of SDFormat 1.8 of course

EricCousineau-TRI

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

I've started prototyping support for nested canonical links in libsdformat9 in scpeters@6c01718, but I'm not sure how much :: syntax we want to backport into SDFormat 1.7 (see the line in the proposal that says <model name="top" canonical_link="nested::link">).

I was very careful to avoid using :: syntax in gazebosim/sdf_tutorials#34, so I'd prefer to avoid it for the 1.7 implementation of gazebosim/sdf_tutorials#36 as well. :: syntax will be part of SDFormat 1.8 of course

OK SGTM per this and Slack convo.

azeey

Reviewable status: complete! all files reviewed, all discussions resolved

Nested model elements (`//model/model`) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#amendment-1-directly-nested-models), this PR adds support for nested models in the DOM API and frame semantics (fixing gazebosim#283) through three steps: * adding `Model::Model*` methods for accessing nested models via the DOM API (047ec96) * loading nested models in `Model::Load` (b57fea2, step 3 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) * supporting nested models (`//model/model`) in frame semantics as opaque frames to match how models in the world scope (`//world/model`) are treated (85e0b4f, steps 6-9 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) The first two steps are straightforward, while the 3rd step adds new behavior. This behavior is added to `libsdformat9` because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. In libsdformat 9.2.0, a `//world/model` supports frame semantics: they have frames that can be referenced by name with `//pose/@relative_to` and `//world/frame/@attached_to` values can resolve to a `//world/model`. This extends that same behavior to nested `//model/model` elements; they now have their own frames in the frame and pose graphs, and a `//model/frame` is permitted to attach to a model. This does not add support for referencing elements within a model via the `::` syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see gazebosim#293). Signed-off-by: Steve Peters <[email protected]>

Nested model elements (`//model/model`) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#amendment-1-directly-nested-models), this PR adds support for nested models in the DOM API and frame semantics (fixing #283) through three steps: * adding `Model::Model*` methods for accessing nested models via the DOM API (047ec96) * loading nested models in `Model::Load` (b57fea2, step 3 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) * supporting nested models (`//model/model`) in frame semantics as opaque frames to match how models in the world scope (`//world/model`) are treated (85e0b4f, steps 6-9 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) The first two steps are straightforward, while the 3rd step adds new behavior. This behavior is added to `libsdformat9` because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. In libsdformat 9.2.0, a `//world/model` supports frame semantics: they have frames that can be referenced by name with `//pose/@relative_to` and `//world/frame/@attached_to` values can resolve to a `//world/model`. This extends that same behavior to nested `//model/model` elements; they now have their own frames in the frame and pose graphs, and a `//model/frame` is permitted to attach to a model. This does not add support for referencing elements within a model via the `::` syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see #293). Signed-off-by: Steve Peters <[email protected]>

Nested model elements (`//model/model`) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#amendment-1-directly-nested-models), this PR adds support for nested models in the DOM API and frame semantics (fixing gazebosim#283) through three steps: * adding `Model::Model*` methods for accessing nested models via the DOM API (047ec96) * loading nested models in `Model::Load` (b57fea2, step 3 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) * supporting nested models (`//model/model`) in frame semantics as opaque frames to match how models in the world scope (`//world/model`) are treated (85e0b4f, steps 6-9 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) The first two steps are straightforward, while the 3rd step adds new behavior. This behavior is added to `libsdformat9` because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. In libsdformat 9.2.0, a `//world/model` supports frame semantics: they have frames that can be referenced by name with `//pose/@relative_to` and `//world/frame/@attached_to` values can resolve to a `//world/model`. This extends that same behavior to nested `//model/model` elements; they now have their own frames in the frame and pose graphs, and a `//model/frame` is permitted to attach to a model. This does not add support for referencing elements within a model via the `::` syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see gazebosim#293). Signed-off-by: Steve Peters <[email protected]>

Nested model elements (`//model/model`) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#amendment-1-directly-nested-models), this PR adds support for nested models in the DOM API and frame semantics (fixing gazebosim#283) through three steps: * adding `Model::Model*` methods for accessing nested models via the DOM API (047ec96) * loading nested models in `Model::Load` (b57fea2, step 3 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) * supporting nested models (`//model/model`) in frame semantics as opaque frames to match how models in the world scope (`//world/model`) are treated (85e0b4f, steps 6-9 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) The first two steps are straightforward, while the 3rd step adds new behavior. This behavior is added to `libsdformat9` because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. In libsdformat 9.2.0, a `//world/model` supports frame semantics: they have frames that can be referenced by name with `//pose/@relative_to` and `//world/frame/@attached_to` values can resolve to a `//world/model`. This extends that same behavior to nested `//model/model` elements; they now have their own frames in the frame and pose graphs, and a `//model/frame` is permitted to attach to a model. This does not add support for referencing elements within a model via the `::` syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see gazebosim#293). An expected console message was updated in a test while merging forward to libsdformat11. Signed-off-by: Steve Peters <[email protected]>

Nested model elements (`//model/model`) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#amendment-1-directly-nested-models), this PR adds support for nested models in the DOM API and frame semantics (fixing #283) through three steps: * adding `Model::Model*` methods for accessing nested models via the DOM API (047ec96) * loading nested models in `Model::Load` (b57fea2, step 3 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) * supporting nested models (`//model/model`) in frame semantics as opaque frames to match how models in the world scope (`//world/model`) are treated (85e0b4f, steps 6-9 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) The first two steps are straightforward, while the 3rd step adds new behavior. This behavior is added to `libsdformat9` because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. In libsdformat 9.2.0, a `//world/model` supports frame semantics: they have frames that can be referenced by name with `//pose/@relative_to` and `//world/frame/@attached_to` values can resolve to a `//world/model`. This extends that same behavior to nested `//model/model` elements; they now have their own frames in the frame and pose graphs, and a `//model/frame` is permitted to attach to a model. This does not add support for referencing elements within a model via the `::` syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see #293). An expected console message was updated in a test while merging forward to libsdformat11. Signed-off-by: Steve Peters <[email protected]>

Nested model elements (`//model/model`) are currently supported in the SDFormat 1.7 spec, but are not supported by the DOM API or frame semantics operations in libsdformat 9.2. Per Amendment 1 of the SDFormat 1.7 proposal (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#amendment-1-directly-nested-models), this PR adds support for nested models in the DOM API and frame semantics (fixing gazebosim#283) through three steps: * adding `Model::Model*` methods for accessing nested models via the DOM API (047ec96) * loading nested models in `Model::Load` (b57fea2, step 3 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) * supporting nested models (`//model/model`) in frame semantics as opaque frames to match how models in the world scope (`//world/model`) are treated (85e0b4f, steps 6-9 of model parsing stages (sdformat.org/tutorials?tut=pose_frame_semantics_proposal#1-model)) The first two steps are straightforward, while the 3rd step adds new behavior. This behavior is added to `libsdformat9` because it is compatible with the SDFormat 1.7 spec (which supports nested models) and makes the treatment of models more consistent with regard to frame semantics. In libsdformat 9.2.0, a `//world/model` supports frame semantics: they have frames that can be referenced by name with `//pose/@relative_to` and `//world/frame/@attached_to` values can resolve to a `//world/model`. This extends that same behavior to nested `//model/model` elements; they now have their own frames in the frame and pose graphs, and a `//model/frame` is permitted to attach to a model. This does not add support for referencing elements within a model via the `::` syntax in the frame semantics or DOM APIs. That will be added in SDFormat 1.8 (see gazebosim#293). Signed-off-by: Steve Peters <[email protected]>

* added to sdf 1.7 in gazebosim#316 Signed-off-by: Steve Peters <[email protected]>

* sdf 1.8: Add <double_sided> to material from #410 * sdf 1.8: Add lightmap to 1.8 spec from #429 * sdf 1.8: document Add L16 camera pixel format from #487 Signed-off-by: Ian Chen <[email protected]> * sdf 1.8: Decrease far clip lower bound from #435 Signed-off-by: Nate Koenig <[email protected]> * sdf 1.8: Added render_order to material from #446 Signed-off-by: ahcorde <[email protected]> * sdf 1.8: Add camera type aliases to docs. from #514 * sdf 1.8: Improve docs of collision_bitmask from #521 Signed-off-by: Martin Pecka <[email protected]> * sdf 1.8: support nested models in @attached_to from #316 Signed-off-by: Steve Peters <[email protected]>

scpeters added this to the SDFormat 1.8 / libsdformat11 milestone Jul 17, 2020

scpeters requested review from azeey, iche033 and EricCousineau-TRI July 17, 2020 01:56

This was referenced Jul 21, 2020

Add support for nested models in TPE gazebosim/gz-physics#86

Merged

Support nested model gazebosim/gz-sim#258

Merged

EricCousineau-TRI reviewed Jul 23, 2020

View reviewed changes

EricCousineau-TRI mentioned this pull request Jul 23, 2020

[epic] proposal: Should provide better composition behavior #278

Closed

17 tasks

EricCousineau-TRI reviewed Jul 23, 2020

View reviewed changes

azeey requested changes Jul 23, 2020

View reviewed changes

iche033 reviewed Jul 23, 2020

View reviewed changes

EricCousineau-TRI mentioned this pull request Jul 23, 2020

composition/proposal: Permit nested canonical links gazebosim/sdf_tutorials#36

Merged

scpeters commented Jul 24, 2020

View reviewed changes

EricCousineau-TRI reviewed Jul 24, 2020

View reviewed changes

scpeters commented Jul 24, 2020

View reviewed changes

EricCousineau-TRI reviewed Jul 24, 2020

View reviewed changes

azeey reviewed Jul 28, 2020

View reviewed changes

EricCousineau-TRI reviewed Jul 30, 2020

View reviewed changes

chapulina assigned scpeters Aug 3, 2020

scpeters commented Aug 6, 2020

View reviewed changes

scpeters added 8 commits August 13, 2020 15:47

sdf/1.7/frame.sdf: allow attaching to models

8fbe7d3

Update the spec to indicate that a frame may be attached to a //world/model or a nested //model/model. Signed-off-by: Steve Peters <[email protected]>

bump to 9.3 and update changelog

3c2d814

Signed-off-by: Steve Peters <[email protected]>

Use more relevant error type

10490d3

Use POSE_RELATIVE_TO_GRAPH_ERROR instead of ELEMENT_INVALID when a valid PoseRelativeToGraph pointer is not available. Signed-off-by: Steve Peters <[email protected]>

remove debugging print statements

5b3b99f

Signed-off-by: Steve Peters <[email protected]>

Add nested model to error messages, update tests

d398d2b

Signed-off-by: Steve Peters <[email protected]>

Add example of joint using unsupported :: syntax

2260869

This syntax is unsupported by DOM objects in SDFormat 1.7. Signed-off-by: Steve Peters <[email protected]>

Add comment referencing gazebosim#293.

9d4fd6d

Signed-off-by: Steve Peters <[email protected]>

Update comments per review

82b03f6

Indicate that Model::Load can also call private method. State that only relative names are supported in graphs in FrameSemantics.hh Signed-off-by: Steve Peters <[email protected]>

scpeters force-pushed the nested_model_dom_9 branch from eee2f64 to 82b03f6 Compare August 13, 2020 22:52

scpeters commented Aug 16, 2020

View reviewed changes

EricCousineau-TRI reviewed Aug 16, 2020

View reviewed changes

scpeters commented Aug 17, 2020

View reviewed changes

EricCousineau-TRI reviewed Aug 17, 2020

View reviewed changes

azeey approved these changes Aug 17, 2020

View reviewed changes

scpeters merged commit 7319836 into gazebosim:sdf9 Aug 17, 2020

scpeters deleted the nested_model_dom_9 branch August 17, 2020 17:42

scpeters mentioned this pull request Aug 17, 2020

Cherry-pick nested model support to sdf10 #338

Merged

scpeters mentioned this pull request Aug 17, 2020

Cherry-pick #312, #316 to master #339

Merged

GiulioRomualdi mentioned this pull request Oct 23, 2020

Error while loading iCubGazeboV3_fixed robotology/icub-models#58

Closed

scpeters added a commit to scpeters/sdformat that referenced this pull request May 18, 2021

sdf 1.8: support nested models in @attached_to

f6f8c28

* added to sdf 1.7 in gazebosim#316 Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request May 18, 2021

Merge sdf 1.7 changes forward to 1.8 #570

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdf9] Support nested models in DOM and frame semantics #316

[sdf9] Support nested models in DOM and frame semantics #316

scpeters commented Jul 17, 2020 •

edited

Loading

codecov-commenter commented Jul 17, 2020 •

edited

Loading

EricCousineau-TRI commented Jul 23, 2020

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

azeey left a comment

iche033 left a comment

EricCousineau-TRI commented Jul 23, 2020

EricCousineau-TRI commented Jul 23, 2020

scpeters left a comment

scpeters left a comment

EricCousineau-TRI left a comment

scpeters left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

azeey left a comment

EricCousineau-TRI left a comment

scpeters left a comment

EricCousineau-TRI commented Aug 6, 2020

scpeters left a comment

EricCousineau-TRI left a comment

scpeters left a comment

EricCousineau-TRI left a comment

azeey left a comment

[sdf9] Support nested models in DOM and frame semantics #316

[sdf9] Support nested models in DOM and frame semantics #316

Conversation

scpeters commented Jul 17, 2020 • edited Loading

codecov-commenter commented Jul 17, 2020 • edited Loading

Codecov Report

EricCousineau-TRI commented Jul 23, 2020

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Jul 23, 2020

EricCousineau-TRI commented Jul 23, 2020

scpeters left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Aug 6, 2020

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

scpeters commented Jul 17, 2020 •

edited

Loading

codecov-commenter commented Jul 17, 2020 •

edited

Loading